-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refine desktop location fetch #989
Conversation
Also, don't fetch location on loading the desktop view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better thank you!
// On desktop, after user clicks "Use location" from the location fields, | ||
// show an alert and explain if location is blocked. | ||
if (!isMobile() && error.code === 1) { | ||
window.alert( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit clunky, but I think good enough for now! Could we possibly add a TODO here to move this code into location-field
and handle this more elgantly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the TODO in 933d34a.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Undoes #984 and instead, displays an alert on desktop when attempting to use the current location for the first time after loading the page, and location access is blocked.
i18n messages are provided when the user denied location access.
PR Checklist